Properly re-own type variables when merging constraints #12519
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The documentation for myOwnedVars used to say that it could contain already instantiated variables because "weak references can have spurious nulls" but I think that's incorrect: the documentation of WeakReference makes no mention of spurious nulls, the object it references should only be replaced by null if it cannot be reached through a non-weak reference. Instead I believe the issue was that `mergeConstraintWith` did not set the `owningState` of type variables that it now owns.
odersky
approved these changes
May 26, 2021
| @@ -161,8 +163,7 @@ class TyperState() { | |||
| constraint.typeVarOfParam(tl.paramRefs(0)) ne other.typeVarOfParam(tl.paramRefs(0)) | |||
| // Note: Since TypeVars are allocated in bulk for each type lambda, we only | |||
| // have to check the first one to find out if some of them are different. | |||
| val conflicting = constraint.domainLambdas.find(tl => | |||
| other.contains(tl) && hasConflictingTypeVarsFor(tl)) | |||
| val conflicting = constraint.domainLambdas.find(constraint.hasConflictingTypeVarsFor(_, other)) | |||
There was a problem hiding this comment.
find not filter? It was like this before, but the logic seems to require a filter. Or am I overlooking something?
There was a problem hiding this comment.
Indeed, I didn't notice, I'll fix it!
After the previous commit a few tests started failing due to TyperStates attempting to instantiate a type variable with an owningState pointing to a different TyperState. The issue is that after `mergeConstraintWith`, multiple TyperState can own the same type variable. This is fine as long as only one of them is committable since the other ones won't attempt to instantiate any type variable, but that's not necessarily the case in FunProto#typedArgs where we merge the constraints of the FunProto context into the passed context. This commit fixes this by instantiating any type variable in the constraints of the FunProto which do not exist in the passed TyperState before calling `mergeConstraintWith`. This ensures that merging can be done without changing the ownership of any type variable, thus keeping both TyperState safely committable.
The use of find instead of filter appears to be a typo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The documentation for myOwnedVars used to say that it could contain
already instantiated variables because "weak references can have
spurious nulls" but I think that's incorrect: the documentation of
WeakReference makes no mention of spurious nulls, the object it
references should only be replaced by null if it cannot be reached
through a non-weak reference. Instead I believe the issue was that
mergeConstraintWithdid not set theowningStateof type variablesthat it now owns.